Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --env-cwd Option To wp-env run #49908

Merged
merged 7 commits into from
Apr 20, 2023
Merged

Add --env-cwd Option To wp-env run #49908

merged 7 commits into from
Apr 20, 2023

Conversation

ObliviousHarmony
Copy link
Contributor

What?

By default all wp-env run commands are ran from /var/www/html. This means that any relative paths used in a command are relative to this directory. This PR adds a new --env-cwd option that changes the working directory for wp-env run.

Why?

In adding support for running PHPUnit tests to our repository, I noticed that running wp-env run tests-wordpress /var/www/html/wp-content/plugins/woocommerce/vendor/bin/phpunit in a script and adding a path to a test file does not work. It expects the relative path to be relative to /var/www/html instead of the phpunit.xml file I use.

As for the name --env-cwd, it's important to note that any options registered by npm or wp-env are consumed by their respective tools instead of being passed on. --cwd seems likely to have been used by some tool, so, we should try and avoid conflict.

I've marked this as a breaking change in case someone is using --env-cwd in their wp-env run commands. Since it's going to consume it there's the possibility it will break for someone.

How?

We're using the -w option provided by docker-compose run to change the working directory of the command. This avoids any messiness with trying to cd.

Testing Instructions

  1. npm run env -- run tests-wordpress pwd should echo /var/www/html.
  2. npm run env -- run tests-wordpress --env-cwd=/var pwd should echo /var.

This option allows `wp-env run` to execute from a
different working directory. This is nice for relative
paths
Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it'd be worthwhile to allow the input to be relative from the WordPress root to reduce noise? E.g. by default, it's resolved from /var/www/html, and then an input of --env-cwd=wp-content/plugins/gutenberg would work. (And if you prefix with "/", it would just pass it directly). I think we could do this with path.resolve

packages/env/README.md Outdated Show resolved Hide resolved
packages/env/lib/cli.js Outdated Show resolved Hide resolved
@ObliviousHarmony
Copy link
Contributor Author

I agree @noahtallen, it makes more sense to use relative paths. I can't really imagine anyone ever using anything other than /var/www/html as a base for a path. I've gone ahead and made that change.

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left a suggestion for the changelog.

Co-authored-by: Noah Allen <noahtallen@gmail.com>
@ObliviousHarmony
Copy link
Contributor Author

Thanks @noahtallen!

🚢

@noahtallen noahtallen enabled auto-merge (squash) April 20, 2023 05:08
@noahtallen noahtallen merged commit bd47ab1 into WordPress:trunk Apr 20, 2023
@github-actions github-actions bot added this to the Gutenberg 15.7 milestone Apr 20, 2023
@noahtallen noahtallen added [Tool] Env /packages/env [Type] Feature New feature to highlight in changelogs. labels Apr 20, 2023
@ObliviousHarmony ObliviousHarmony deleted the add/env-cwd-option branch April 20, 2023 23:22
@bph bph added [Type] Enhancement A suggestion for improvement. and removed [Type] Feature New feature to highlight in changelogs. labels Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] Env /packages/env [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants